Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make directRenderScript the default #11791

Merged
merged 4 commits into from
Aug 28, 2024
Merged

Make directRenderScript the default #11791

merged 4 commits into from
Aug 28, 2024

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Aug 20, 2024

Changes

Make directRenderScript the default and removes previous hoisted script handling.

Testing

The tests should pass. I edited most of the tests to not test against hoisting behaviour anymore, and to not mention the word "hoisted"

Docs

Would appreciate review of the changesets in the PR!

On the docs repo side, this will require a migration guide, which users have to check that their scripts are still working as expected. The general docs may also need to be updated to not reference the "hoisting to head" behaviour of scripts. (Question: should I commit this other change to the v5-beta branch directly, or via PR?)

Copy link

changeset-bot bot commented Aug 20, 2024

🦋 Changeset detected

Latest commit: e996921

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added pkg: astro Related to the core `astro` package (scope) pr: docs A PR that includes documentation for review labels Aug 20, 2024
@bluwy bluwy marked this pull request as draft August 20, 2024 15:25
@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Aug 21, 2024
@bluwy bluwy marked this pull request as ready for review August 21, 2024 13:55
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the red parts of the diff

Copy link
Member

@sarah11918 sarah11918 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bluwy , super amazing changeset here!

As for your questions, anything on the upgrade guide page itself, feel free to commit directly! Yes, we'll need this kind of advice, so we should probably put a breaking changes entry in for this one (even though I've already covered the general unflagging of all the newly stable things, this one needs extra guidance).

For more general updates throughout the docs, a PR is great because it calls more attention (to me) as to what kinds of changes are necessary, and are more likely to stick in my mind as I basically have to go through ALL the docs for all kinds of new stuff everywhere 😅 Would love for you to go through and PR some updates, and then even if you don't catch all of them, we have an idea of what kind of changes to be on the lookout for in case we spot something that isn't as obvious and doesn't return a direct search hit.

.changeset/five-jars-hear.md Outdated Show resolved Hide resolved
Co-authored-by: Sarah Rainsberger <[email protected]>
@bluwy bluwy merged commit 9393243 into next Aug 28, 2024
5 checks passed
@bluwy bluwy deleted the plt-2086-direct-render-script branch August 28, 2024 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs pr pkg: astro Related to the core `astro` package (scope) pkg: integration Related to any renderer integration (scope) pr: docs A PR that includes documentation for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants